Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better loading indicator #43

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

A-Guldborg
Copy link
Contributor

Attempted to fix #39
However, there are two issues:

  1. It only supports LoaderContent and ChildContent, where the first is what is shown while loading and the latter while not loading. This means that we have to move any input fields outside of our MudLoading component if we want to show it all the time (such as input fields, otherwise they will "reload" on input change). I am not a stylist/webdesigner, so this also means some of our pages changes slightly, see screenshots below.
  2. It does not seem to work well with our MudTable in our UserTable after I "pulled" the input field outside the MudTable in order to not have this reload every time we search for something, but after searching for a user it ends up being stuck on the loading icon. Likely, this is because of the ServerData not working with the input field outside or something along those lines, but I did not have more time to investigate this.

I like that we can add the loading content thing "prettier" and an if-statement, but I don't think it saves us any complexity. With the below changes in mind, and the differences in code from the previous versions and this, I suggest we consider whether we want to pursue this further. Consider this a quick proof of concept (even though it does not prove 100%), and feel more than free to scratch it if you don't think it adds the right value.

Changes

Our Statistics page changed from:
image
To:
image

Our Manage User page changed from:
image
To:
image
(And still does not work)

@A-Guldborg A-Guldborg added enhancement New feature or request needs discussion Needs discusion to further elaborate the requirements labels Apr 23, 2024
@A-Guldborg A-Guldborg requested a review from TTA777 April 23, 2024 21:30
@A-Guldborg A-Guldborg self-assigned this Apr 23, 2024
Copy link
Member

@TTA777 TTA777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that in the current version, it's not very desirable. My initial thought was that we would wrap MudLoading in our current LoadingIndicator component, so the syntax for using it would be something like :

<LoadingIndicator Loading="_loading" Height ="400px">
    <WhatEverChildContent>
    </WhatEverChildContent>
</LoadingIndicator >

Doing so would both make the syntax for using it a lot more concise, and avoid unnecessary duplication.

As for the changes in the layout, I'm not sure why they are necessary without sitting a bit with it. If you want to revisit this, either by yourself or with me. Let me know. Otherwise, I can also try to draft something up

@A-Guldborg
Copy link
Contributor Author

I agree that in the current version, it's not very desirable. My initial thought was that we would wrap MudLoading in our current LoadingIndicator component, so the syntax for using it would be something like :

<LoadingIndicator Loading="_loading" Height ="400px">
    <WhatEverChildContent>
    </WhatEverChildContent>
</LoadingIndicator >

Doing so would both make the syntax for using it a lot more concise, and avoid unnecessary duplication.

As for the changes in the layout, I'm not sure why they are necessary without sitting a bit with it. If you want to revisit this, either by yourself or with me. Let me know. Otherwise, I can also try to draft something up

We can definitely look at it together, or you can draft something up before "pair-programming", but I think if you include the MudLoading part in our LoadingIndicator.razor there will probably be issues hiding the data while loading (if we want that). But let's discuss in person :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs discussion Needs discusion to further elaborate the requirements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate if MudLoading can be used with our own loading animation
2 participants